Conversation
9851ba0 to
5987457
Compare
|
@PragTob hope you will find some time to review the changes. |
5987457 to
9a8b324
Compare
|
@PragTob ping! |
1 similar comment
|
@PragTob ping! |
2ce910b to
54684e8
Compare
54684e8 to
8ede952
Compare
33cbb8e to
574b45c
Compare
There was a problem hiding this comment.
Thanks for your work on this PR, it would make a welcome inclusion 🙏
I am curious - would method coverage improve coverage support for ruby 3's endless methods?
I have found that endless methods currently show as covered if they are never executed by tests.
Even if you have branching coverage enabled they report as fully covered.
Example class, spec and coverage report follow.
Note that uncovered_endless_method shows as covered despite not being executed by the test.
Whereas endless_branching_method shows branching coverage failing since it was executed.
I would have expected uncovered_endless_method to show as partially covered.
class CoveragePoc
# this method is fully covered
def self.simple_method
:simple
end
# the :b branch for these methods is not covered
def self.endless_branching_method(arg) = arg ? :a : :b
def self.branching_method(arg)
arg ? :a : :b
end
# these methods are not covered at all
def self.uncovered_endless_method = :uncovered_endless
def self.uncovered_method
:uncovered
end
endRSpec.describe CoveragePoc do
it 'works' do
expect(described_class.simple_method).to eq(:simple)
expect(described_class.branching_method(true)).to eq(:a)
expect(described_class.endless_branching_method(true)).to eq(:a)
end
end|
Hey @amatsuda, maybe you could take a look? I know there are conflicts, but I don't want to fix them in case there are no plans to merge this PR anytime soon. |
|
@tycooon Thank you for working on this! Yes, I'm very much interested in this feature, and willing to take a look once the patch is cleanly rebased against the main branch :) |
c2e0130 to
c59387a
Compare
|
Hey @amatsuda, I just updated the branch 👍 |
| end | ||
|
|
||
| def adapt_old_style_branch_info(value) | ||
| eval(value.to_s) # rubocop:disable Security/Eval |
There was a problem hiding this comment.
I know this eval was in the original code, but I’d love to remove it.
sferik
left a comment
There was a problem hiding this comment.
@tycooon thanks for all your work on this PR! It's a great feature idea. I reviewed the code and left some minor feedback.
Reviewing this inspired me to take a crack at an alternative implementation that builds on the same ideas but makes a few different tradeoffs in add-method-coverage-support.
Here are main differences between my approach and yours:
- Keeps string keys (
"lines","branches","methods") instead of converting to symbols. This avoids the breaking change for downstream consumers likerubycriticthat accesscoverage_datadirectly, and avoids touching nearly every spec file. ResultMergeris completely untouched. The memory optimization of deferringResultobject creation until the final merge step is preserved. Method data flows through the existing combine pipeline naturally sinceFilesCombiner/MethodsCombineroperate on raw hashes just likeLinesCombiner/BranchesCombinerdo.- Replaces
evalwith a safe StringScanner-based parser inrestore_ruby_data_structure. This addresses #801 without requiring a separateResultSerializationclass. The same parser handles both branch and method data keys after JSON round-tripping. - Memory address normalization happens in
ResultAdapter(early, before the JSON round-trip inResult.new) rather than during serialization.
Everything else is the same in spirit: the SourceFile::Method class, MethodsCombiner, configuration via enable_coverage :method, FileList aggregation, etc.
What do you think?
|
|
||
| def total_methods | ||
| @total_methods ||= covered_methods + missed_methods | ||
| end |
There was a problem hiding this comment.
The total_methods method defined in FileList returns a count, as the name suggests, but this one returns an array. Please change the name of this one (or change its behavior to be consistent with the other).
| end | ||
|
|
||
| describe "method coverage" do | ||
| it "has total methods count 0" do |
There was a problem hiding this comment.
| it "has total methods count 0" do | |
| it "has total methods count 3" do |
| expect(subject.total_methods.size).to eq(3) | ||
| end | ||
|
|
||
| it "has covered methods count 0" do |
There was a problem hiding this comment.
| it "has covered methods count 0" do | |
| it "has covered methods count 2" do |
| expect(subject.covered_methods.size).to eq(2) | ||
| end | ||
|
|
||
| it "has missed methods count 0" do |
There was a problem hiding this comment.
| it "has missed methods count 0" do | |
| it "has missed methods count 1" do |
| end | ||
|
|
||
| def to_s | ||
| "#{klass}##{method}" |
There was a problem hiding this comment.
Ruby’s Coverage.result uses different prefixes for instance methods vs class methods (e.g., #<Class:Foo> for singleton methods). It should probably use using . instead of # for class methods.
| end | ||
|
|
||
| def lines_data | ||
| coverage_data.fetch(:lines) |
There was a problem hiding this comment.
Is it possible for the :lines key to be missing (e.g. if the user just requests method coverage data)? If so, this will crash.
| arm64-darwin | ||
| x86_64-darwin | ||
| x86_64-linux-gnu | ||
| x86_64-linux-musl |
There was a problem hiding this comment.
This shouldn’t be part of this PR.
|
Hey @sferik, I like your approach more than mine. I am closing this PR for now 👍 Also don't forget to add some support for HTML formatter. Example of what I did is here simplecov-ruby/simplecov-html#110. And btw since you added your own parser, maybe we can address #916 as I did in the same branch? |

See #782.
Also addresses #801 and #916.
Support for HTML formatter can be found here: simplecov-ruby/simplecov-html#110.
Some notes:
ResultSerializationmodule).ResultMergeroptimizations :( Now it's operatingResultobjects, however it still parses and merges files one be one.lines: []instead of"lines" => []), just the same way you get it when calling Ruby'sCoverage.result. Because of that, a lot of specs were updated.0x0000000000000000.adapt_pre_simplecov_0_18_resultlogic was moved toResultSerialization.deserialize. Also old-style branch info is supported (evalis still used for that).All this stuff has been battle-tested on a couple of big projects, some of which merge results of multiple CI jobs.